fix(precompiles): fail closed on native coin burn supply underflow - #333

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so any review state I set (approval or change request) carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review.

Reviewed the diff and traced the reachability question. The change is correct and I'd merge it — but as defence-in-depth, not as a bug fix. The failure mode in the description isn't reachable on any current Arc network, and I think that's worth establishing explicitly, because it decides whether this needs a hardfork gate.

The supply invariant is seeded exactly at genesis

TOTAL_SUPPLY_STORAGE_KEY is slot 2 on NATIVE_COIN_AUTHORITY_ADDRESS. Both shipped genesis files set it to precisely the sum of allocated balances:

So total_supply == sum(balances) holds at block 0 by construction, not by accident.

Every balance-mutating path preserves it

I grepped for all native-balance mutation sites across crates/evm and crates/precompiles:

• mint — supply += amt then balance_incr(amt). Preserved.
• burn — balance_decr(amt) then supply -= amt. Preserved.
• transfer — balance_decr + balance_incr, explicitly net-zero (native_coin_authority.rs:332). Supply untouched, correctly.
• Fees are a pure transfer, not a burn. reward_beneficiary (handler.rs:85-118) credits basefee + priority to the beneficiary, with the comment "This overrides the default EIP-1559 behavior which burns the base fee." The caller is debited by the standard revm path, so the fee is a move, not a destruction. Preserved.
• system_accounting.rs and native_coin_control.rs — zero balance mutations. Neither touches native balances at all.

The one path that does break the invariant breaks it the safe way

handler.rs:103-111 documents it: crediting a self-destructed beneficiary "silently burns the fee at commit." Zero8 rejects that condition, but pre-Zero8 it can happen — balances drop while supply stays put.

That drives total_supply above sum(balances). Underflow needs the opposite drift. So the only known invariant-breaking mechanism in the tree makes checked_sub strictly less likely to fail, never more.

Which makes the burn precondition airtight

balance_decr has already succeeded when the subtraction runs, so:

amount <= balance(from) <= sum(balances) <= total_supply

checked_sub cannot return None. The deleted comment's conclusion was right — though its stated reason ("due to the balance check") was incomplete. The balance check alone only gives amount <= balance(from); you need the global invariant for the second step. If you keep a comment here, that's the version worth writing down, since it's the part a future reader can't re-derive locally.

The part I'd want a maintainer to rule on: gating

This changes a state-transition outcome — a burn that previously succeeded with supply saturating to 0 now reverts and rolls back. That's consensus-observable if it can ever trigger, and the PR adds it ungated.

Ungated is the right call only because it's unreachable. That's a real constraint, not a formality — this codebase gates every observable behavior change in these precompiles:

• native_coin_control.rs:132,146,250,264 — Zero8
• helpers.rs:574,608 — Zero8
• system_accounting.rs:660,668,670 — Zero5/Zero6
• handler.rs:105 — the selfdestructed-beneficiary guard above, Zero8

So the PR can't have it both ways: if the invariant genuinely can lag (the premise in the description), this is a consensus change and needs a ArcHardfork gate like its neighbours. If it can't lag, the change is inert and safe ungated — but then "burn could silently zero supply" overstates it, and the PR risks being triaged as a security fix when it's hardening.

I'd suggest rewording the description to something like "unreachable under the total_supply == sum(balances) invariant; added as fail-closed hardening, therefore not hardfork-gated" — so a reviewer doesn't have to re-derive the reachability argument to approve it.

On the ERR_OVERFLOW reuse

@kutluhaneth46's point that this matches mint's error surface is right, but I checked the other uses and there's no existing precedent for it on an underflow path — both are genuine overflows:

• helpers.rs:444 — TransferError::OverflowPayment
• helpers.rs:486 — checked_add overflow in balance_incr

helpers.rs already exports ERR_INSUFFICIENT_FUNDS, which is closer to what a supply shortfall actually is. Not worth blocking over — consistency with mint is a defensible tiebreak — but it's a new semantic for the constant rather than an established one.

Minor while you're here: ERR_OVERFLOW is defined twice with identical text — helpers.rs:51 (pub) and native_coin_authority.rs:57 (local, shadowing the shared one). The local could just be dropped in favour of the import.

Test

Good instinct putting this in crates/ as a Rust unit test — that actually executes in Public CI under cargo nextest. Worth knowing that the tests/**/*.test.ts hardhat suites are invoked only by make test-unit-hardhat and no workflow calls them, so a regression test placed there would never have run.

The note about not pinning gas_used is correct. One addition worth making: the test asserts the revert reason but not the rollback. Since "fail closed" is the actual claim, asserting that total_supply is still 0 and ADDRESS_A's balance is unchanged afterwards would pin the behaviour that matters, rather than just the error string.

Not requesting changes — the code is right and the test is real. The reachability framing and the gating question are what I'd want resolved in the description before merge. Usual caveat: no cargo or rustc available here, so this is source review plus arithmetic on the committed genesis JSON, not a test run.